feat: track phase changes in sync coordinator #168
Conversation
WalkthroughAdds detection of phase name changes in the sync coordinator's network monitoring loop by tracking the last emitted phase name and including phase changes in the progress emission condition. Changes
Sequence Diagram(s)sequenceDiagram
participant Monitor as NetworkMonitorLoop
participant Snapshot as PhaseSnapshot
participant Emitter as ProgressEmitter
Note over Monitor: loop: observe height/headers/filters + phase
Monitor->>Snapshot: read phase_snapshot
Snapshot-->>Monitor: phase_snapshot.name()
Note right of Monitor: compute current_phase_name\ncompare with last_emitted_phase_name
alt phase_changed or other metrics changed
Monitor->>Emitter: emit progress (height/headers/filters/phase)
Emitter-->>Monitor: ack
Note right of Monitor: update last_emitted_phase_name
else no relevant change
Monitor-->>Monitor: continue loop
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
dash-spv/src/client/sync_coordinator.rs (1)
399-399: Correct state update with minor optimization opportunity.The update to
last_emitted_phase_nameis correct and necessary for tracking phase changes across iterations. However, sincecurrent_phase_nameis not used after this line, the.clone()is unnecessary.Consider removing the unnecessary clone for a minor optimization:
- last_emitted_phase_name = Some(current_phase_name.clone()); + last_emitted_phase_name = Some(current_phase_name);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dash-spv/src/client/sync_coordinator.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
dash-spv/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/**/*.rs: Enforce Rust formatting viacargo fmt --checkon all Rust source files
All code must be clippy-clean: runcargo clippy --all-targets --all-features -- -D warnings
Files:
dash-spv/src/client/sync_coordinator.rs
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Use proper error types with thiserror and propagate errors appropriately
Use the tokio runtime for async operations in Rust
Use conditional compilation feature flags for optional features (#[cfg(feature = ...)])
Format Rust code with cargo fmt (and enforce via cargo fmt --check)
Run clippy with -D warnings and fix all lints
Adhere to MSRV Rust 1.89 (avoid features requiring newer compiler)
**/*.rs: Format Rust code with rustfmt (per rustfmt.toml); run cargo fmt --all before commits
Lint with clippy; treat warnings as errors in CI
Follow Rust naming: snake_case for functions/variables, UpperCamelCase for types/traits, SCREAMING_SNAKE_CASE for consts
Prefer async using tokio where applicable
Files:
dash-spv/src/client/sync_coordinator.rs
**/src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/src/**/*.rs: Each crate keeps sources in src/
Avoid unwrap()/expect() in library code; use proper error types (e.g., thiserror)
Place unit tests alongside code with #[cfg(test)]
Files:
dash-spv/src/client/sync_coordinator.rs
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Coordinate synchronization phases with `SequentialSyncManager`; each phase must complete before the next begins
📚 Learning: 2025-08-25T17:15:59.361Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Coordinate synchronization phases with `SequentialSyncManager`; each phase must complete before the next begins
Applied to files:
dash-spv/src/client/sync_coordinator.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (dash_deserialize_amount)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: RPC Tests (stable, true)
- GitHub Check: Strict Warnings and Clippy Checks
- GitHub Check: Core Components Tests
- GitHub Check: SPV Components Tests
- GitHub Check: Key Wallet Components Tests
🔇 Additional comments (3)
dash-spv/src/client/sync_coordinator.rs (3)
119-119: LGTM! Phase tracking variable properly initialized.The initialization of
last_emitted_phase_nametoNoneis correct and ensures that the initial phase will be emitted on the first status update, since the comparison will evaluate totruewhen comparingNoneagainst the first phase name.
367-369: LGTM! Phase change detection logic is correct.The comparison correctly detects phase transitions:
- Initial phase emission:
None != Some(¤t_phase_name)evaluates totrue- Unchanged phase:
Some(&prev) != Some(&prev)evaluates tofalse- Phase change:
Some(&prev) != Some(&new)evaluates totrueThis ensures progress is emitted when synchronization phases change, as intended by the PR.
374-374: LGTM! Emission condition properly extended.Adding
|| phase_changedto the emission condition ensures that progress updates are triggered when the synchronization phase changes, even if heights and filter counts remain unchanged. This achieves the PR's objective of accurately reflecting phase transitions in emitted progress.
|
warnings are unrelated in clippy due to clippy updates; not introduced in this PR. |
Added functionality to monitor phase changes in the sync coordinator, enhancing the progress emission logic. This update ensures that changes in synchronization phases are accurately reflected in the emitted progress, improving the overall synchronization process.
PastaPastaPasta
force-pushed
the
fix/emit-event-phase-change
branch
from
7d0c8ab to
6f2fb6c
Compare
|
|
Added functionality to monitor phase changes in the sync coordinator, enhancing the progress emission logic. This update ensures that changes in synchronization phases are accurately reflected in the emitted progress, improving the overall synchronization process.
Summary by CodeRabbit